Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add flyte-core missing priorityClassName to webhook values #4987

Merged
merged 1 commit into from
Mar 27, 2024
Merged

Add flyte-core missing priorityClassName to webhook values #4987

merged 1 commit into from
Mar 27, 2024

Conversation

ddl-ebrown
Copy link
Contributor

@ddl-ebrown ddl-ebrown commented Feb 29, 2024 •
edited
Loading

Tracking issue

https://github.com/flyteorg/flyte/issues/

Why are the changes needed?

  • All the other pods can have their priorityClassName set except for the webhook

What changes were proposed in this pull request?

  • New config setting for webhook priorityClassName like other services

How was this patch tested?

The updated chart is being used to deploy Flyte in our environment. There are tests that validate that priorityClassName propagates to the webhook pod properly

Setup process

Screenshots

Check all the applicable boxes

  • I updated the documentation accordingly.
  • All new and existing tests passed.
  • All commits are signed-off.

Related PRs

Docs link

@dosubot dosubot bot added size:S This PR changes 10-29 lines, ignoring generated files. enhancement New feature or request labels Feb 29, 2024
@ddl-ebrown ddl-ebrown force-pushed the add-webhook-priorityClassName branch from 27ccb07 to 5d3336a Compare February 29, 2024 22:51
@ddl-ebrown ddl-ebrown changed the title Adds missing priorityClassName to webhook values Add flyte-core missing priorityClassName to webhook values Feb 29, 2024
@ddl-ebrown ddl-ebrown force-pushed the add-webhook-priorityClassName branch from 5d3336a to cdca47a Compare February 29, 2024 22:53
@codecov Codecov
Copy link

codecov bot commented Mar 1, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 58.99%. Comparing base (b6f35ad) to head (95e8294).

Additional details and impacted files 
@@           Coverage Diff           @@
##           master    #4987   +/-   ##
=======================================
  Coverage   58.99%   58.99%           
=======================================
  Files         645      645           
  Lines       55648    55648           
=======================================
  Hits        32831    32831           
  Misses      20222    20222           
  Partials     2595     2595
Flag Coverage Δ
unittests 58.99% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pingsutw
pingsutw approved these changes Mar 4, 2024
View reviewed changes
Copy link
Member

@pingsutw pingsutw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you, @ddl-ebrown. Could you update the PR description?

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Mar 4, 2024
pingsutw
pingsutw previously approved these changes Mar 4, 2024
View reviewed changes
@ddl-ebrown
Copy link
Contributor Author

LGTM, thank you, @ddl-ebrown. Could you update the PR description?

What part of the description did you need updated?

@pingsutw
Copy link
Member

pingsutw commented Mar 5, 2024 •
edited
Loading

@ddl-ebrown, it's good now. Last time I saw it, it was empty. Maybe it was Github issue.

@ddl-ebrown ddl-ebrown force-pushed the add-webhook-priorityClassName branch from c1cd31e to d0ccb27 Compare March 6, 2024 06:43
davidmirror-ops
davidmirror-ops previously approved these changes Mar 13, 2024
View reviewed changes
Copy link
Contributor

@davidmirror-ops davidmirror-ops left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Thanks!

@ddl-ebrown ddl-ebrown force-pushed the add-webhook-priorityClassName branch 2 times, most recently from 76a414f to 48f5050 Compare March 15, 2024 03:20
@dosubot dosubot bot added size:XS This PR changes 0-9 lines, ignoring generated files. and removed size:S This PR changes 10-29 lines, ignoring generated files. labels Mar 15, 2024
@ddl-ebrown
Copy link
Contributor Author

PR updated to remove conflicts, so this should be good to go.

@ddl-ebrown
Copy link
Contributor Author

Looks like there's a new process for bumping the chart version number?

@ddl-ebrown
Add flyte-core missing webhook priorityClassName
95e8294 
 - All the other pods can have their priorityClassName set except for
   the webhook

Signed-off-by: ddl-ebrown <[email protected]>
@ddl-ebrown ddl-ebrown force-pushed the add-webhook-priorityClassName branch from 48f5050 to 95e8294 Compare March 22, 2024 20:26
@ddl-ebrown
Copy link
Contributor Author

@davidmirror-ops repushed this one as well. Hopefully can get this across the line soon.

davidmirror-ops
davidmirror-ops approved these changes Mar 27, 2024
View reviewed changes
Copy link
Contributor

@davidmirror-ops davidmirror-ops left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once again, thanks so much

@davidmirror-ops davidmirror-ops merged commit 7c57d51 into flyteorg:master Mar 27, 2024
53 checks passed
@ddl-ebrown ddl-ebrown deleted the add-webhook-priorityClassName branch March 28, 2024 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidmirror-ops davidmirror-ops davidmirror-ops approved these changes

@pingsutw pingsutw pingsutw left review comments

Assignees
No one assigned
Labels
enhancement New feature or request lgtm This PR has been approved by a maintainer size:XS This PR changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ddl-ebrown @pingsutw @davidmirror-ops